-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix all standardrb errors and warnings #168
Conversation
$ docker compose run stocks bin/rails db:create | ||
$ docker compose run stocks bin/rails test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, useful
def show; end | ||
def show | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the single lines aesthetically, but I'm not going to fight the standardrb defaults over it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D
Now you're feeling that "unconfigurable configuration" vibe.
@@ -1,5 +1,5 @@ | |||
class Classroom < ApplicationRecord | |||
belongs_to :year | |||
belongs_to :school | |||
has_many :users | |||
has_many :users, dependent: :nullify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought through the alternative arguments to dependent:
and I think this is the correct one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also suspect we may end up with a join model here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Yeah, I can see that
app/models/stock.rb
Outdated
has_many :portfolio_stocks | ||
has_many :orders | ||
has_many :portfolio_stocks, dependent: :nullify | ||
has_many :orders, dependent: :nullify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels wrong to me because stocks should never be deleted. They should be moved to an inactive
state instead. Maybe :restrict_with_error
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it.
"should never be deleted" will have a lot more interesting considerations in the future, but this is an easy fix now
app/models/portfolio.rb
Outdated
has_many :portfolio_transactions | ||
has_many :portfolio_stocks | ||
has_many :portfolio_transactions, dependent: :destroy | ||
has_many :portfolio_stocks, dependent: :destroy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may also be a candidate for restrict_with_error
or restrict_with_exception
I like autoformatters because they help me have a second chance to look at the code with a fresh perspective in addition to their other benefits. This looks good to me. I especially like all the README updates |
This is something we can pick back up after the event. Closing for now, can reopen a new PR later. |
In order to establish a safe base for auto-formatting using editor-based tooling with the Standard formatter, I want to clean up all existing
standardrb
andstandardrb-rails
errors and warnings.I did this in two stages:
bundle exec standardrb --fix
This way we can add "Ruby Lint" as a required check for all Pull Requests going forward.